crypto crate implementation #17

amika-sq · 2023-11-21T19:39:50Z

This PR is a first pass at an implementation for the crypto crate.

This implementation utilized the ssi_jwk and ssi_jws crates from Spruce's excellent SSI library: https://github.com/spruceid/ssi/

The structure & operations here follows that in web5-kt/crypto with a few notable exceptions, which I have outlined below. I am open to changing/renaming any of these!

`PrivateKey` and `PublicKey`

There is an additional abstraction over JWKs: PrivateKey and PublicKey. This allows us to move signing / verifying closer to the actual objects doing the work.

Spruce's ssi library utilizes JWKs as the key type for most of it's operations, so these abstractions directly wrap a JWK. In the future, if we want to introduce another key type, we can convert these into generic traits.

This abstraction also helps with the uniffi-rs binding generation process which we will be using for Swift, which will be coming in a later PR. Because we don't own the JWK type (it's defined in ssi_jwk), we can't directly expose it to the binding generator without duplicating it. Using Rust newtypes that wrap the JWK allows us to get around this.

`LocalKeyManager` and `KeyStore`

Our ultimate goal here is to export this Rust code into Swift code. On Swift, we want a way to generate cryptographic keys via Rust, but then store them using secure storage mechanisms such as the Keychain and the Secure Enclave. In order to accomodate this, I needed to split apart the "key generation" and "key storage" tasks into separate traits.

LocalKeyManager is how I did this. It's responsibilities are to generate the cryptographic keys locally (locally meaning in Rust) and then delegate the actual storage to a KeyStore trait.

I have provided a InMemoryKeyStore implementation that stores the cryptographic keys in memory. But anyone can provide their own implementation, including foreign languages.

crates/crypto/src/key/key.rs

decentralgabe · 2023-11-21T20:20:46Z

crates/crypto/src/key_manager/local_key_manager.rs

+            .get(key_alias)?
+            .ok_or(KeyManagerError::SigningKeyNotFound)?;
+
+        let signed_payload = private_key.sign(&payload.to_vec())?;


is it assumed that all key types expose this sign method?

That's correct. The PrivateKey struct is currently responsible for all key types, which itself is a thin wrapper around a JWK.

Definitely flexible here to alternative approaches!

I think it may be worth including a signing algorithm parameter, as sometimes the same key can be used with different algorithms (e.g. secp256k1 can be used with ECDH and ECDSA)

This should still be possible, by the caller setting the algorithm field on the JWK.

Currently, the algorithm is computed by this function in Spruce's ssi-jwk crate:
https://github.com/spruceid/ssi/blob/main/ssi-jwk/src/lib.rs#L341

This logic matches our signer lookup that's currently being done in Kotlin here:
https://github.com/TBD54566975/web5-kt/blob/main/crypto/src/main/kotlin/web5/sdk/crypto/Crypto.kt#L111C22-L111C22

crates/crypto/src/key_manager/key_store/in_memory_key_store.rs

andresuribe87

Forgot to hit send a bit back...

andresuribe87 · 2023-11-21T21:42:24Z

crates/crypto/src/key_manager/local_key_manager.rs

+/// is being run. Key storage is provided by a [`KeyStore`] trait implementation, allowing the keys
+/// to be stored wherever is most appropriate for the application.
+pub struct LocalKeyManager {
+    key_store: Arc<dyn KeyStore>,


I'm not sure I understand the ownership story here for KeyStore and LocalKeyManager, would you mind elaborating on how it's supposed to work?

Tangentially, for my learning, do you know when you are expected to use Arc::clone?

Arc is a shared data reference that uses the concept of reference counting. When you create an Arc via Arc::new() around some piece of data, that data is placed on the heap along with two counters for "strong" and "weak" references to it. After Arc::new(), it looks something like this:

The Arc you get back is actually a pointer to some heap allocated memory (called ArcInner), which contains the counts, as well as the actual shared data itself. If the Arc pointer you get back goes out of scope and gets deallocated, the strong reference counter that the ArcInner was pointing to is atomically decremented. When the strong count falls to zero, the heap allocated memory that contains the actual data is then cleaned up.

Now looking at these particular lines:

pub struct LocalKeyManager { key_store: Arc<dyn KeyStore>, }

What this is saying is that LocalKeyManager owns an Arc, which points to a KeyStore. The lifetime of the Arc will only end when the LocalKeyManager's lifetime ends. This means that the strong reference count will remain non-zero for the duration of LocalKeyManager's lifetime, guaranteeing that it's always available to use.

In order to construct a LocalKeyManager, the caller must to provide an Arc<dyn Keystore> value in the constructor. This can be done either by creating a new Arc like I mentioned earlier with Arc::new(), or the caller can clone an existing Arc.

Cloning an Arc isn't actually cloning the heap allocated data. Instead, it's cloning the pointer to that heap allocated data. As a part of the clone operation, the strong reference count is atomically incremented. So, even if the originally cloned Arc goes out of scope and gets deallocated, decrementing the reference count, the reference count is still non-zero.

I hope that made some sort of sense 😅 . Happy to chat through this more if you'd like more clarity, or had additional questions! Arc is definitely one of the more interesting data types in Rust, and it's heavily used throughout the bindings generation process which will come in future PRs.

Thanks! Asked more in the line below.

crates/crypto/src/key_manager/key_store/in_memory_key_store.rs

mistermoe

🏅

andresuribe87 · 2023-11-27T16:49:50Z

crates/crypto/src/key_manager/local_key_manager.rs

+impl LocalKeyManager {
+    /// Constructs a new `LocalKeyManager` that stores keys in the provided `KeyStore`.
+    pub fn new(key_store: Arc<dyn KeyStore>) -> Self {
+        Self { key_store }


Is this a place where you should be using ::clone on the passed in key_store? If not, mind elaborating why?

No, not exactly. The contract here says "I need an Arc<dyn KeyStore>." It's the caller's responsibility to provide that. Whether that's done by a ::clone(), ::new(), or moving an existing Arc is an implementation detail that the caller must handle. If they do not, it's a compilation error.

andresuribe87 · 2023-11-27T16:51:49Z

crates/crypto/src/key_manager/mod.rs

@@ -0,0 +1,7 @@
+mod key_manager;
+pub use key_manager::*;


Feels like wildcard is difficult to maintain. I typically recommend being explicit on what this is exposing.

Tangentially, would it make sense to setup a linter? I briefly looked online and found https://github.com/rust-lang/rust-clippy

This is pretty common in Rust library development. It's just saying that I can use the key_manager module without specifying key_manager again.

Without this line, caller's use statements would look like this:
use crypto::key_manager::key_manager::KeyManager

But with this line, callers can reference it like so:
use crypto::key_manager::KeyManager

andresuribe87 · 2023-11-27T16:52:35Z

crates/crypto/src/key_manager/local_key_manager.rs

+/// is being run. Key storage is provided by a [`KeyStore`] trait implementation, allowing the keys
+/// to be stored wherever is most appropriate for the application.
+pub struct LocalKeyManager {
+    key_store: Arc<dyn KeyStore>,


Thanks! Asked more in the line below.

crates/crypto/src/key/private_key.rs

andresuribe87 · 2023-11-27T18:57:34Z

crates/crypto/Cargo.toml

+[package]
+name = "crypto"
+version = "0.1.0"
+edition = "2021"


Every once in a while, there's a change to the Rust language that isn't backwards compatible. Editions are how that's conveyed in the Cargo.toml file. 2021 was the latest change to language, and we are just declaring that we're compatible with versions from that point on.

More info on editions here! https://doc.rust-lang.org/edition-guide/editions/

crates/crypto/src/key_manager/key_store/in_memory_key_store.rs

KendallWeihe

Nice! This is cool. I'm a total Rust noob so don't feel qualified to give an official ✅

jiyoonie9 · 2023-11-27T20:00:21Z

crates/crypto/src/key_manager/key_manager.rs

+/// Implementations of this trait might provide key management through various Key Management
+/// Systems (KMS), such as AWS KMS, Google Cloud KMD, Hardware Security Modules (HSM), or simple
+/// in-memory storage, each adhering to the same consistent API for usage within applications.
+pub trait KeyManager: Send + Sync {


i noticed you have : Send + Sync bits here, and i think these traits have to do with being able to pass types across threads, and share references between threads.

i don't know enough about rust to fully understand the docs but could you tell me why you chose to add these two traits? is it because we need to use Arc in all key manager's implementations?

Excellent question! It's actually to do with exposing this trait for external bindings via uniffi (which will be upcoming in a future PR). There's a requirement that exposed traits MUST implement Send + Sync, because which thread it comes across isn't guaranteed. There's a bit of explanation about it here: https://mozilla.github.io/uniffi-rs/udl/interfaces.html#exposing-traits-as-interfaces

So, for now, it's mostly for things yet to come!

jiyoonie9

i learned a lot by just reading this PR and the comments! thanks for writing this 😊

crypto crate implementation

d2c8f4c

amika-sq requested review from michaelneale, mistermoe, leordev, nitro-neal, ethan-tbd, diehuxx and frankhinek November 21, 2023 19:41

decentralgabe reviewed Nov 21, 2023

View reviewed changes

crates/crypto/src/key/key.rs Outdated Show resolved Hide resolved

decentralgabe reviewed Nov 21, 2023

View reviewed changes

ethan-tbd requested a review from jiyoonie9 November 21, 2023 21:03

rename KeyAlgorithm -> KeyType

d21fa36

amika-sq requested a review from andresuribe87 November 21, 2023 21:09

ethan-tbd reviewed Nov 22, 2023

View reviewed changes

crates/crypto/src/key_manager/key_store/in_memory_key_store.rs Outdated Show resolved Hide resolved

andresuribe87 reviewed Nov 22, 2023

View reviewed changes

Mutex -> RwLock

a7e5e16

amika-sq force-pushed the crypto-crate branch from af7e435 to a7e5e16 Compare November 22, 2023 20:14

mistermoe approved these changes Nov 22, 2023

View reviewed changes

andresuribe87 reviewed Nov 27, 2023

View reviewed changes

jiyoonie9 reviewed Nov 27, 2023

View reviewed changes

crates/crypto/src/key/private_key.rs Show resolved Hide resolved

andresuribe87 approved these changes Nov 27, 2023

View reviewed changes

jiyoonie9 reviewed Nov 27, 2023

View reviewed changes

crates/crypto/src/key_manager/key_store/in_memory_key_store.rs Show resolved Hide resolved

KendallWeihe reviewed Nov 27, 2023

View reviewed changes

jiyoonie9 reviewed Nov 27, 2023

View reviewed changes

amika-sq merged commit d6fe02a into main Nov 27, 2023

amika-sq deleted the crypto-crate branch November 27, 2023 20:23

This was referenced Nov 27, 2023

implement secp256k1 private key generation, signing and verification #14

Closed

implement Ed25519 private key generation, signing and verification #15

Closed

Credentials crate & PresentationDefinitionV2 struct definition #18

Merged

This was referenced Dec 1, 2023

Create and resolve did:jwk #20

Merged

Rename KeyType -> KeyAlgorithm #34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto crate implementation #17

crypto crate implementation #17

amika-sq commented Nov 21, 2023 •

edited

Loading

decentralgabe Nov 21, 2023

amika-sq Nov 21, 2023

decentralgabe Nov 21, 2023

amika-sq Nov 27, 2023

andresuribe87 left a comment

andresuribe87 Nov 21, 2023

amika-sq Nov 22, 2023

andresuribe87 Nov 27, 2023

mistermoe left a comment

andresuribe87 Nov 27, 2023

amika-sq Nov 27, 2023 •

edited

Loading

andresuribe87 Nov 27, 2023

amika-sq Nov 27, 2023 •

edited

Loading

andresuribe87 Nov 27, 2023

andresuribe87 Nov 27, 2023

amika-sq Nov 27, 2023

KendallWeihe left a comment

jiyoonie9 Nov 27, 2023

amika-sq Nov 27, 2023

jiyoonie9 left a comment

crypto crate implementation #17

crypto crate implementation #17

Conversation

amika-sq commented Nov 21, 2023 • edited Loading

PrivateKey and PublicKey

LocalKeyManager and KeyStore

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresuribe87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mistermoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amika-sq Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amika-sq Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KendallWeihe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiyoonie9 left a comment

Choose a reason for hiding this comment

amika-sq commented Nov 21, 2023 •

edited

Loading

`PrivateKey` and `PublicKey`

`LocalKeyManager` and `KeyStore`

amika-sq Nov 27, 2023 •

edited

Loading

amika-sq Nov 27, 2023 •

edited

Loading